Add Pylint checker and test for unescaped underscore symbols in docstrings - #8514

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.59%. Comparing base (8a8fc5c) to head (aba8ba4).

@@           Coverage Diff            @@
##             main    #8514    +/-   ##
========================================
  Coverage   99.58%   99.59%            
========================================
  Files        1137     1139     +2     
  Lines      105119   105392   +273     
========================================
+ Hits       104687   104961   +274     
+ Misses        432      431     -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Scope Question: The checker runs on every file pylint lints, including *_test.py files and private (_-prefixed) helpers, whose docstrings don't ever seem to appear in the API reference. 11 of the 13 files with docstring fixes in this PR are test files. Do we prefer to:

(a) keep it uniform as is, which is simpler and consistent with the repo's other docstring lint rules, or

(b) skip test files so it only enforces where rendering matters?

Happy to do either; with (b), the test-file edits can stay as cleanup.

Hi @BretMishler – thank you for tackling this!

Before going much further, I have two higher-level requests/feedback.

1. I think it would be better to split out the fixes to existing docstrings into a separate PR, leaving the present PR to be only about tooling. Although the inclusion of updated docstrings can serve to demonstrate the results of applying the code (and perhaps that was the motivation), it's preferable to separate the concerns, and it's also better aligned with "small CLs" principles. The unit tests that are part of the new code already provide a demonstration, so this part will not really be lost; plus, each PR's description can reference the other one, to help connect them more explicitly.

2. Implementing a self-contained parser based on low-level AST primitives and regexes is a hard road to take, fraught with potholes. If Gemini is to be believed, the current implementation also has a number of flaws in the regexes as well as doing things like re-parsing and re-tokenizing every file. So, I did some digging into alternatives, and rather than continuing down this road, I suggest following another approach may be simpler and more maintainable. (Full disclosure: Gemini did the heavy lifting here.) It uses a combination of the astroid package and Pylint's BaseChecker class to leverage existing machinery. An untested implementation produced by Gemini is attached to this comment; it may not be flawless, but may at least point in a viable direction and provide an initial starting point.

docstring-underscores-gemini-untested.py

Whoops, I missed your questions earlier:

Scope Question: The checker runs on every file pylint lints, including *_test.py files and private (_-prefixed) helpers, whose docstrings don't ever seem to appear in the API reference. 11 of the 13 files with docstring fixes in this PR are test files. Do we prefer to:

(a) keep it uniform as is, which is simpler and consistent with the repo's other docstring lint rules, or

(b) skip test files so it only enforces where rendering matters?

Though it's going to be more annoying for human devs, I'd favor option (a). More uniformity ⟹ fewer exceptions to remember ⟹ less cognitive load.

@pavoljuhas @arettig What do you think? (For speed, you can also simply can use thumbs up/down to indicate agreement/disagreement.)

1. I think it would be better to split out the fixes to existing docstrings into a separate PR, leaving the present PR to be only about tooling.

+1

...
2. An untested implementation produced by Gemini is attached to this comment; it may not be flawless, but may at least point in a viable direction and provide an initial starting point.

docstring-underscores-gemini-untested.py

It may be worthwhile to look at the implementation of docstring checks in the pylint itself - for example, invalid-characters-in-docstring is checked via class SpellingChecker(BaseTokenChecker), which on a quick look seems to already have parsing and processing of AST nodes.

Perhaps we could reuse that machinery, at very least the BaseTokenChecker class?

Also, if getting the exact col_offset of an unescaped string involves too much pain, we could do with reporting just the line number. There are not that many positives in this PR and after fix this checker should have zero output.